Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use gRPC to query connection end #1048

Merged
merged 4 commits into from
Jun 14, 2021
Merged

Use gRPC to query connection end #1048

merged 4 commits into from
Jun 14, 2021

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Jun 4, 2021 •
edited by romac
Loading

Partial fix for: #1001

Description

This PR changes the query_connection implementation from querying the Tendermint RPC to querying the Cosmos SDK GRPC directly.

Because of cosmos/ibc-go#149, the height parameter is removed from query_connection. However this is fine, as shown in the diff, since all calls to query_connection other than the command line always use the latest height of the blockchain. As a result the change should not break any existing behavior.

We will need to implement retry logic any way in case a target height of interest have not been reach on the blockchain. For querying into the past, we can wait for cosmos/ibc-go#149 to be fixed first.

This fix is in parallel to #1042. The change to use GRPC call should make it less urgent to the fix by #1042. But handling uninitialized fields in protobuf3 is still an outstanding problem that needs to be solved eventually.

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@romac romac changed the title Use GRPC to query connection end Use gRPC to query connection end Jun 7, 2021
romac
romac requested changes Jun 7, 2021
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a small nitpick and a question for @ancazamfir.

relayer/src/error.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/connection.rs Outdated
Comment on lines 26 to 27
#[options(help = "height of the state to query", short = "h")]
height: Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ancazamfir Is that ok with you, or do you sometimes rely on being able to query the connection end at specific heights?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need to query specific heights (coming in the connection worker PR). And for this reason we cannot use the gRPC query.

ancazamfir
ancazamfir requested changes Jun 7, 2021
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue that describes the problem we are trying to fix here? This might be ok to be used by the CLIs only. But for relaying we do need to be able to query connection at height, take proofs at given height, etc. This is currently not supported by SDK gRPC implementation and for this reason we cannot use gRPC at this point (see cosmos/ibc-go#149). Also we want to be able to test the APIs the relayer needs via the CLIs. This is why many queries have the -h parameter that might look awkward.

relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/connection.rs Outdated
Comment on lines 26 to 27
#[options(help = "height of the state to query", short = "h")]
height: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need to query specific heights (coming in the connection worker PR). And for this reason we cannot use the gRPC query.

@soareschen
Copy link
Contributor Author

Is there an issue that describes the problem we are trying to fix here?

As discussed in #1042, doing this query over Tendermint RPC is not very reliable, because when the connection is not found, the RPC returns empty buffer instead of proper error code. So the long term solution is still to do the query over GRPC when possible.

yes, we need to query specific heights (coming in the connection worker PR)

Perhaps we can come back to this PR again when the connection worker PR is ready, so that we have a good use case of what is currently lacking on the GRPC API and push for the fix to be done sooner.

@adizere
Copy link
Member

adizere commented Jun 9, 2021 •
edited
Loading

Goal:

  • revert the APIs to have height param
  • use gRPC with height param using @andynog's proposed solution
  • implement the whole method inside impl Chain for CosmosSDK
  • fix Romain's comment
  • leave method in the impl Chain for CosmosSDK
    • Chain trait will ideally evolve to rely on gRPC calls

Future:

  • think about refactoring the block_on awkward parts into separate async fn blocks

@soareschen soareschen force-pushed the soares/grpc-query-connection branch from 24209ff to a7ad50b Compare June 14, 2021 10:48
@soareschen
Copy link
Contributor Author

I have now added back the height parameter using the x-cosmos-block-height header as documented at https://docs.cosmos.network/master/run-node/interact-node.html#query-for-historical-state-using-grpcurl.

@romac romac requested a review from ancazamfir June 14, 2021 11:55
ancazamfir
ancazamfir approved these changes Jun 14, 2021
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks @soareschen!

@romac romac merged commit 3c7cd0f into master Jun 14, 2021
@romac romac deleted the soares/grpc-query-connection branch June 14, 2021 13:05
@soareschen soareschen mentioned this pull request Jun 14, 2021
Open
5 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@soareschen
Use gRPC to query connection end (informalsystems#1048)
3147eb9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancazamfir ancazamfir ancazamfir approved these changes

@romac romac romac approved these changes

@adizere adizere Awaiting requested review from adizere

Assignees

@soareschen soareschen

@ancazamfir ancazamfir

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soareschen @adizere @romac @ancazamfir